Switch from `android_glue` to `ndk-glue` and remove outdated lifecycle handing #1411

jamienicol · 2022-05-27T13:50:11Z

Closes #1307, closes #1313, closes #1334

Additionally remove the Android lifecycle handling code, as winit no longer provides the required callback meaning it does not compile.

This fixes compilation on Android. However, there is a caveat: Glutin is no longer able to automatically destroy and create the Surface in response to Android lifecycle events. The application must therefore ensure they only create a Context following a winit Event::Resumed, and they destroy the context when receiving an Event::Suspended.

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
cargo doc builds successfully
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality

MarijnS95 · 2022-05-27T13:57:29Z

However, there is a caveat: Glutin is no longer able to automatically destroy and create the Surface in response to Android lifecycle events. The application must therefore ensure they only create a Context following a winit Event::Resumed, and they destroy the context when receiving an Event::Suspended.

For the record: I'll PR my example as soon as I get home that showcases how this is done. And also for the record, we're working on changes to make this more ergonomic too.

MarijnS95 · 2022-05-27T14:02:35Z

glutin/src/api/android/mod.rs

-            // 'on_surface_destroyed' Android event can arrive with some delay
-            // because multithreading communication. Because of
-            // that, swap_buffers can be called before processing
-            // 'on_surface_destroyed' event, with the native window
-            // surface already destroyed. EGL generates a BAD_SURFACE error in
-            // this situation. Set stop to true to prevent
-            // swap_buffer call race conditions.


Ah we "fixed" this in ndk-glue but it relies on the caller/user (winit) to hold a lock on the window and release it after its done all its cleanup upon receiving WindowDestroyed. Looks like at least winit's fn raw_window_handle() -> RawWindowHandle doesn't do that yet, but new_windowed() here calls directly into ndk_glue::native_window() where we can implement that 🎉

It should perhaps be part of this PR though (to store nwin inside the context).

It looks like #1320 reported this too, which we can probably close as "outdated" when this is merged.

Fwiw the API is currently annoying with an Option inside the lock, I'm thinking about changing ndk-glue to parking_lot which allows mapping - in this case specifically for transmuting - the Option out of the RWLockReadGuard.

Putting the RwLockGuard in AndroidContext fails to compile because it is not Sync:

Compiling glutin v0.28.0 (/home/jamie/src/glutin/glutin) error[E0277]: `std::sync::RwLockReadGuard<'static, Option<NativeWindow>>` cannot be sent between threads safely --> glutin/src/context.rs:204:6 | 204 | impl FailToCompileIfNotSendSync for Context<NotCurrent> {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^ `std::sync::RwLockReadGuard<'static, Option<NativeWindow>>` cannot be sent between threads safely | = help: within `AndroidContext`, the trait `Send` is not implemented for `std::sync::RwLockReadGuard<'static, Option<NativeWindow>>` = note: required because it appears within the type `Option<std::sync::RwLockReadGuard<'static, Option<NativeWindow>>>` note: required because it appears within the type `AndroidContext` --> glutin/src/api/android/mod.rs:18:8 | 18 | struct AndroidContext { | ^^^^^^^^^^^^^^ = note: required because of the requirements on the impl of `Sync` for `Arc<AndroidContext>` note: required because it appears within the type `api::android::Context` --> glutin/src/api/android/mod.rs:25:12 | 25 | pub struct Context(Arc<AndroidContext>); | ^^^^^^^ note: required because it appears within the type `context::Context<context::NotCurrent>` --> glutin/src/context.rs:34:12 | 34 | pub struct Context<T: ContextCurrentState> { | ^^^^^^^ note: required by a bound in `FailToCompileIfNotSendSync` --> glutin/src/context.rs:201:18 | 199 | trait FailToCompileIfNotSendSync | -------------------------- required by a bound in this 200 | where 201 | Self: Send + Sync, | ^^^^ required by this bound in `FailToCompileIfNotSendSync`

Unless I'm misunderstanding your suggestion? Any ideas how to fix?

You're not misunderstanding my suggestion, it's just a little oversight that I haven't checked further than "this lock guard must be held in some way" and now that you actually try it, see that glutin requires its context to be Send/Sync.

My gut feeling says that glutin relies on the (EGL) context is internally synchronized (hence safe to not only Send to a different thread, but also access from multiple threads concurrently (Sync) without locking on the Rust side). A lock guard is obviously not, so it seems like we'll have to wrap the lock guard inside a Mutex 🤭. Logically that doesn't solve the requirement for Send though, maybe those locks from parking_lot can be moved into a different thread?

For reference, the nomicon nicely explains why lock guards are not Send:

https://doc.rust-lang.org/nomicon/send-and-sync.html

A nice example where this does not happen is with a MutexGuard: notice how it is not Send. The implementation of MutexGuard uses libraries that require you to ensure you don't try to free a lock that you acquired in a different thread.

Looks like parking_lot supports sending a lock guard though, as long as we enable send_guard: https://github.com/Amanieu/parking_lot#usage

At this point it feels like we might put this issue on ice, and solve it when we get to it - as I'd like to address it in winit as well and anyway need parking_lot to be able to map a lock guard and get rid of the inner Option<>.

EDIT: If we do skip this (I'm out of ideas bar migrating ndk-glue to parking_lot), please put a TODO comment in the code here to explain that we should really hold on to the lock even if the context is moved to a different thread.

@jamienicol Here you go:

rust-mobile/ndk#282
jamienicol/glutin@ndk-glue...MarijnS95:ndk-glue-native-window-lock

We'll need to figure out something with the versioning - I had to do some patch hacks to get everything in the dependency chain (winit) to use the same ndk and ndk-glue. But that's after everyone is content with this idea :)

Fwiw I've added this to winit in rust-windowing/winit#2307 to make the lock held at least as long as until after winit users processed and returned from Event::Suspended. With that in this becomes less of an issue but it still feels right to hold the lock at exactly the same place where the "consumer" of the window - an EGL window surface - is lifecycle-managed. However, given that we don't really help the user yet with lifetime management given the removal of set_suspend_callback, we should just get your PR merged without solving this one and I'll take it on later.

I've been having even more thoughts about this (rust-windowing/winit#2307 (comment)), and I don't think glutin should hold the lock for this bit of the API but depend on winit to do it (rust-windowing/winit#2307).

Then, with that squared away (though we can just "trust" the surface is safe for now since that's what everyone is doing and works "okay enough", until the next winit release with my PR in) glutin doesn't - shouldn't - use ndk_glue directly at all. winit uses raw-window-handle to expose these details, and glutin can simply use it directly:

MarijnS95@baaf31a

(I've added the locks to winit with that in mind: people will use that raw-window-handle interface and never see nor know about the Android lock directly).

In fact we can use this to our advantage to unify glutin to use this "generic" way to communicate window-handle pointers around and create GL contexts on it, that's how it's done for Vulkan at least: https://github.com/ash-rs/ash/tree/master/ash-window (out of scope for this PR).

glutin/src/api/android/mod.rs

MarijnS95

Other than the unwrap() nit and request to hold the NativeWindow lock I'm totally happy with this PR!

glutin/src/api/android/mod.rs

MarijnS95 · 2022-05-28T17:54:01Z

@jamienicol As part of this PR, can you also update the ### Android section in README.md? Just spotted that it still links to the old android-rs-glue repo.

…guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)

glutin/Cargo.toml

rib · 2022-05-29T01:00:12Z

If you're looking at switching the glue layer, I wonder if it could be worth evaluating these glue crates that I've been working on recently, while I've been investigating how to support native Rust applications that aren't always based on NativeActivity.

https://github.com/rib/agdk-rust

The glue API was initially based on ndk-glue but it notably handles native <-> Java synchronization internally so there's no need to e.g. hold a lock over the internal window state for correct synchronization.

The API also supports input events, and exposes things like the sdk_version, internal/external/obb_data_dir without needing to depend on ANativeActivity or AInputQueue so there's more opportunity to support alternative Activity base classes.

MarijnS95 · 2022-05-29T19:56:30Z

@rib Let's tackle one thing at a time. This patch and some subsequent changes are working well for its purpose, and are simply necessary to accompany glutins upgrade to newer winit, which wasn't tested on Android nor is caught by the CI (but I will change that once this patch lands).

winit in its current form uses ndk-glue, and winit is tied "heavily" into glutin, hence it is not possible to randomly use a different native/game-activity wrapper here.

Leave the sales-pich for winit and glutin can follow.

rib · 2022-05-30T08:59:17Z

@rib Let's tackle one thing at a time. This patch and some subsequent changes are working well for its purpose, and are simply necessary to accompany glutins upgrade to newer winit, which wasn't tested on Android nor is caught by the CI (but I will change that once this patch lands).

Yep, absolutely, makes sense. Definitely not meaning to disrupt fixing issues that exist now.

winit in its current form uses ndk-glue, and winit is tied "heavily" into glutin, hence it is not possible to randomly use a different native/game-activity wrapper here.

Leave the sales-pich for winit and glutin can follow.

haha, :) sorry for sounding like a sales pitch. I mainly just wanted to float the idea, with the specific technical context that the API in native-activity and game-activity offers a potential solution to this synchronization problem.

There's also room for incorporating a similar approach in ndk-glue and my intention behind suggesting to "evaluate" my work was to try and tone down being hard sale pitch vs pointing to a potential technical solution too (but yeah when I read it back it does look like a sales pitch, sorry about that!)

jamienicol · 2022-05-30T09:03:18Z

@MarijnS95 I've updated the link in the readme and added the comment to the Cargo.toml file. And rebased and fixed the changelog conflict.

Do we need to do anything with the native window lock in this PR? or do we need to wait for your winit PR to land, and update the winit dep in glutin first?

jamienicol · 2022-05-30T09:09:31Z

Perhaps I should combine the two Android changelog entries to emphasise what the users will actually care about, eg:

Fix compilation on Android:

Switch from StaticStructGenerator to StructGenerator to dynamically load symbols.

Replace android_glue dependency with ndk-glue, and remove broken lifecycle event handling.

Glutin can now be used on Android, however, the application must ensure it only creates the Context following a winit Event::Resumed event, and destroy the context in response to a Event::Suspended event.

How does that sound?

README.md

MarijnS95 · 2022-05-30T09:21:40Z

@jamienicol Sounds good on the suggested changelog regrouping, I love some sub-ul's :)

MarijnS95 · 2022-05-30T09:23:16Z

Do we need to do anything with the native window lock in this PR? or do we need to wait for your winit PR to land, and update the winit dep in glutin first?

Let's leave it like this for now. I'll probably plan a larger change that removes ndk-glue again in favour of using raw_window_handle, which should work across all platforms and isn't wrapped in a lock anyway.

CHANGELOG.md

Additionally remove the Android lifecycle handling code, as winit no longer provides the required callback meaning it does not compile. This fixes compilation on Android. However, there is a caveat: Glutin is no longer able to automatically destroy and create the Surface in response to Android lifecycle events. The application must therefore ensure they only create a Context following a winit Event::Resumed, and they destroy the context when receiving an Event::Suspended.

MarijnS95 · 2022-05-30T09:34:26Z

@rib:

haha, :) sorry for sounding like a sales pitch.

Hehe you may have noticed I have a habit for spamming ideas and walls of text in PRs too (side-eye rust-windowing/winit#2307) 😁

I mainly just wanted to float the idea, with the specific technical context that the API in native-activity and game-activity offers a potential solution to this synchronization problem.

It's a good idea to conjure up something that solves this broadly for the generic RawWindowHanle case, tying into the discussion of making winit more consistent across all platforms wrt Event::Resumed/Event::Suspended etc.

There's also room for incorporating a similar approach in ndk-glue and my intention behind suggesting to "evaluate" my work was to try and tone down being hard sale pitch vs pointing to a potential technical solution too (but yeah when I read it back it does look like a sales pitch, sorry about that!)

It's all good, just that glutin doesn't - shouldn't - use any of the input queues, native activity, data dir stuff, that's more winits place. As mentioned in the long suggestion thread above, if we use RawWindowHandle in glutin it can be made oblivious to what's backing the window - and we can even use "generic" Java Surfaces (oddly named NativeWindow in the NDK) that don't need to represent the entire window crated by/for NativeActivity: #1324 (comment)

MarijnS95

🥳

MarijnS95 · 2022-05-30T22:26:04Z

It's all good, just that glutin doesn't - shouldn't - use any of the input queues

How embarrassing; I just read the title of this project in the README and IN in glutIN apparently stands for INPUT 🤭

I thought glutin merely juggled the various OpenGL contexts around, and it's probably only exposing input through the reexported EventLoop from winit nowadays 😬

(And that in any case has little effect on agdk-rust, it's all internal to winit and shouldn't leak out to glutin)

…guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)

…guards (#282) It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)

jamienicol mentioned this pull request May 27, 2022

Use non-static StructGenerator on Android #1398

Merged

7 tasks

MarijnS95 changed the title ~~Switch from android_glue to ndk-glue~~ Switch from android_glue to ndk-glue May 27, 2022

MarijnS95 reviewed May 27, 2022

View reviewed changes

glutin/src/api/android/mod.rs Outdated Show resolved Hide resolved

MarijnS95 approved these changes May 27, 2022

View reviewed changes

MarijnS95 changed the title ~~Switch from android_glue to ndk-glue~~ Switch from android_glue to ndk-glue and remove outdated lifecycle handing May 27, 2022

MarijnS95 reviewed May 27, 2022

View reviewed changes

glutin/src/api/android/mod.rs Outdated Show resolved Hide resolved

jamienicol force-pushed the ndk-glue branch from 53f59f2 to 232c69e Compare May 27, 2022 15:02

MarijnS95 mentioned this pull request May 28, 2022

ndk-glue: Switch to parking_lot for mappable and Sendable lock guards rust-mobile/ndk#282

Merged

MarijnS95 reviewed May 28, 2022

View reviewed changes

glutin/Cargo.toml Outdated Show resolved Hide resolved

MarijnS95 added the platform: Android label May 29, 2022

This was referenced May 29, 2022

android: Hold NativeWindow lock until after notifying the user with Event::Suspended rust-windowing/winit#2307

Merged

Run clippy in the CI with warnings disallowed; address most warnings #1412

Merged

jamienicol force-pushed the ndk-glue branch 2 times, most recently from c0222a9 to a1557f3 Compare May 30, 2022 09:01

jamienicol force-pushed the ndk-glue branch from a1557f3 to 740532e Compare May 30, 2022 09:17

MarijnS95 reviewed May 30, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

MarijnS95 approved these changes May 30, 2022

View reviewed changes

jamienicol force-pushed the ndk-glue branch 2 times, most recently from afbb6d4 to 008e5c2 Compare May 30, 2022 09:25

MarijnS95 reviewed May 30, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

jamienicol force-pushed the ndk-glue branch from 008e5c2 to 946c1c3 Compare May 30, 2022 09:26

MarijnS95 approved these changes May 30, 2022

View reviewed changes

MarijnS95 merged commit 2239712 into rust-windowing:master May 30, 2022

jamienicol deleted the ndk-glue branch May 30, 2022 10:01

MarijnS95 mentioned this pull request Jun 5, 2022

Add Android example #1417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from `android_glue` to `ndk-glue` and remove outdated lifecycle handing #1411

Switch from `android_glue` to `ndk-glue` and remove outdated lifecycle handing #1411

jamienicol commented May 27, 2022 •

edited by MarijnS95

Loading

MarijnS95 commented May 27, 2022 •

edited

Loading

MarijnS95 May 27, 2022 •

edited

Loading

MarijnS95 May 27, 2022

MarijnS95 May 27, 2022

jamienicol May 27, 2022

MarijnS95 May 27, 2022 •

edited

Loading

MarijnS95 May 27, 2022 •

edited

Loading

MarijnS95 May 28, 2022

MarijnS95 May 28, 2022

MarijnS95 May 28, 2022

MarijnS95 May 29, 2022

MarijnS95 left a comment

MarijnS95 commented May 28, 2022

rib commented May 29, 2022 •

edited

Loading

MarijnS95 commented May 29, 2022

rib commented May 30, 2022

jamienicol commented May 30, 2022

jamienicol commented May 30, 2022

MarijnS95 commented May 30, 2022

MarijnS95 commented May 30, 2022

MarijnS95 commented May 30, 2022 •

edited

Loading

MarijnS95 left a comment

MarijnS95 commented May 30, 2022 •

edited

Loading

Switch from android_glue to ndk-glue and remove outdated lifecycle handing #1411

Switch from android_glue to ndk-glue and remove outdated lifecycle handing #1411

Conversation

jamienicol commented May 27, 2022 • edited by MarijnS95 Loading

MarijnS95 commented May 27, 2022 • edited Loading

MarijnS95 May 27, 2022 • edited Loading

Choose a reason for hiding this comment

MarijnS95 May 27, 2022

Choose a reason for hiding this comment

MarijnS95 May 27, 2022

Choose a reason for hiding this comment

jamienicol May 27, 2022

Choose a reason for hiding this comment

MarijnS95 May 27, 2022 • edited Loading

Choose a reason for hiding this comment

MarijnS95 May 27, 2022 • edited Loading

Choose a reason for hiding this comment

MarijnS95 May 28, 2022

Choose a reason for hiding this comment

MarijnS95 May 28, 2022

Choose a reason for hiding this comment

MarijnS95 May 28, 2022

Choose a reason for hiding this comment

MarijnS95 May 29, 2022

Choose a reason for hiding this comment

MarijnS95 left a comment

Choose a reason for hiding this comment

MarijnS95 commented May 28, 2022

rib commented May 29, 2022 • edited Loading

MarijnS95 commented May 29, 2022

rib commented May 30, 2022

jamienicol commented May 30, 2022

jamienicol commented May 30, 2022

MarijnS95 commented May 30, 2022

MarijnS95 commented May 30, 2022

MarijnS95 commented May 30, 2022 • edited Loading

MarijnS95 left a comment

Choose a reason for hiding this comment

MarijnS95 commented May 30, 2022 • edited Loading

Switch from `android_glue` to `ndk-glue` and remove outdated lifecycle handing #1411

Switch from `android_glue` to `ndk-glue` and remove outdated lifecycle handing #1411

jamienicol commented May 27, 2022 •

edited by MarijnS95

Loading

MarijnS95 commented May 27, 2022 •

edited

Loading

MarijnS95 May 27, 2022 •

edited

Loading

MarijnS95 May 27, 2022 •

edited

Loading

MarijnS95 May 27, 2022 •

edited

Loading

rib commented May 29, 2022 •

edited

Loading

MarijnS95 commented May 30, 2022 •

edited

Loading

MarijnS95 commented May 30, 2022 •

edited

Loading